Skip to content

Conversation

BlueLort
Copy link

@BlueLort BlueLort commented Aug 27, 2024

Original PR: #103

Copying description here.

Issue description:

When we assert RemoteResourceType and RemoteResourceIdentifier attributes in EMF logs, we are using Logs filter: RemoteService and RemoteOperation filter to fetch logs. However, it's not guaranteed that EMF log with RemoteService and RemoteOperation attributes will have RemoteResourceType and RemoteResourceIdentifier. The logs fetched with this generic filter might get more logs than expected and only asserting the first log event in the result.

Description of changes:

This change enhances the log filter by appending && ($.RemoteResourceType = %EXAMPLE%) && ($.RemoteResourceIdentifier = %EXAMPLE%) to the filter if they present in the template to get more specific log events.

Log Filter doc: https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/FilterAndPatternSyntax.html#regex-expressions

Log Filter used before this change (From E2E testing log):

2024-07-02T13:41:46.9400277Z 13:41:46.840 [main] INFO  com.amazon.aoc.validators.CWLogValidator - Filter Pattern for Log Search: { ($.Service = sample-application-eu-west-1-9762031914-112) && ($.Operation = "GET /aws-sdk-call") && ($.RemoteService = "AWS::S3") && ($.RemoteOperation = "GetBucketLocation") }

Log Filter used after this change (From E2E testing log):

2024-07-02T13:49:00.6162597Z 13:49:00.510 [main] INFO  com.amazon.aoc.validators.CWLogValidator - Filter Pattern for Log Search: { ($.Service = sample-application-eu-west-1-9762033464-113) && ($.Operation = "GET /aws-sdk-call") && ($.RemoteService = "AWS::S3") && ($.RemoteOperation = "GetBucketLocation") && ($.RemoteResourceType = %^AWS::S3::Bucket$%) && ($.RemoteResourceIdentifier = %^e2e-test-bucket-name-eu-west-1-9762033464-113$%) }

e2e test run: https://github.com/ektabj/aws-application-signals-test-framework/actions/runs/10580614526/job/29315930502 on us-east-1 thanks to Ekta.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@majanjua-amzn majanjua-amzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the change taking effect in the test run you provided: link

If I'm just missing it, could you point it out?

EDIT: Meant to delete this comment before sending out the review comments

}

if (remoteResourceType != null && remoteResourceIdentifier != null) {
dependencyFilter += String.format(" && ($.RemoteResourceType = %%%s%%) && ($.RemoteResourceIdentifier = %%%s%%)", remoteResourceType, remoteResourceIdentifier);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we expect to be searching with the value directly, we can likely just remove the ^...$ RegEx symbols in the validation templates and remove the need for RegEx here altogether. Not a 100% necessary change but doing so will improve the search time by filtering for the exact string instead of a RegEx pattern.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Mahad, we shouldn't expect the same value for either remoteResourceType or remoteResourceIdentifier. You can verify this in the following run: GitHub Workflow Run.

  1. The /aws-sdk-call API shows different values.
  2. The /mysql API also has different values.

I believe these values can't be static since some of them are concatenated into a string only during runtime. Let me know if I've missed anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ektabj

I think Mahad suggests to not use regex here.

As you can see, we are not using regex for RemoteResourceType in the template.
For RemoteResourceIdentifier, we can change the value from ^{{remoteResourceIdentifier}}$ to {{remoteResourceIdentifier}} in the template.

Template: https://github.com/aws-observability/aws-application-signals-test-framework/blob/main/validator/src/main/resources/expected-data-template/java/eks/rds-mysql-log.mustache#L34-L35

@BlueLort
Copy link
Author

I don't see the change taking effect in the test run you provided: link

If I'm just missing it, could you point it out?

Sorry, maybe i'm missing something on my end, but I think I can see the change in effect for example these two log lines:

Searching for expected log: {[\"EC2.AutoScalingGroup\"]=^eks-.+, [\"EC2.InstanceId\"]=^i-[A-Za-z0-9]{17}$, [\"EKS.Cluster\"]=^e2e-canary-test$, Environment=^eks:e2e-canary-test/ns-10580614526-3$, [\"K8s.Namespace\"]=^ns-10580614526-3$, [\"K8s.Node\"]=^i-[A-Za-z0-9]{17}$, [\"K8s.Pod\"]=^sample-app-deployment-java-eks-10580614526-3-1(-[A-Za-z0-9]*)*$, [\"K8s.Workload\"]=^sample-app-deployment-java-eks-10580614526-3-1$, PlatformType=^AWS::EKS$, Service=^sample-application-java-eks-10580614526-3-1$, Operation=GET /aws-sdk-call, Version=^1$, RemoteService=AWS::S3, RemoteOperation=GetBucketLocation, RemoteResourceIdentifier=^e2e-test-bucket-name-java-eks-10580614526-3-1$, RemoteResourceType=^AWS::S3::Bucket$, [\"Telemetry.Source\"]=^ClientSpan$, Host=^ip(-[0-9]{1,3}){4}.*$}
15:07:03.168 [main] INFO  com.amazon.aoc.validators.CWLogValidator - Value of an actual log: ..
..
PlatformType=AWS::EKS, RemoteDbUser=***, RemoteOperation=SELECT, RemoteResourceIdentifier=information_schema|rdsaurorajavaclusterformysql.cluster-clcbvosbkffd.us-east-1.rds.amazonaws.com|3306, RemoteResourceType=DB::Connection, RemoteService=mysql, Service=sample-application-java-eks-10580614526-3-1, 

I see link is tagged with a line with log "Filter Pattern for Log Search" which wasn't changed in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants